Skip to content

Remove peak_sign in favour of main_channel_index#4624

Draft
chrishalcrow wants to merge 24 commits into
SpikeInterface:mainfrom
chrishalcrow:less_peak_sign_more_main_channel
Draft

Remove peak_sign in favour of main_channel_index#4624
chrishalcrow wants to merge 24 commits into
SpikeInterface:mainfrom
chrishalcrow:less_peak_sign_more_main_channel

Conversation

@chrishalcrow

@chrishalcrow chrishalcrow commented Jun 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Sequel to #4374

I didn't have permission to push to Sam's branch, so I made my own...

Idea is that when you make your analyzer, it must have a peak_sign, peak_mode and hence a main_channel_index. We then use these throughout the codebase instead of propagating the other stuff.

@samuelgarcia did the tedious work, but now I'm refining and getting the tests to pass...

WIP!

samuelgarcia and others added 12 commits February 6, 2026 16:59
@chrishalcrow chrishalcrow added core Changes to core module postprocessing Related to postprocessing module labels Jun 23, 2026
@chrishalcrow chrishalcrow changed the title Remove peak_sign in favour of main_channel Remove peak_sign in favour of main_channel_index Jun 23, 2026
chrishalcrow
chrishalcrow commented Jun 24, 2026
View reviewed changes
Comment thread src/spikeinterface/core/analyzer_extension_core.py
extension_name = "templates"
depend_on = ["random_spikes|waveforms"]
need_recording = True
need_recording = False

@chrishalcrow chrishalcrow Jun 24, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have waveforms, you do not need the Recording. Not sure how to deal with this.

@alejoe91 alejoe91 Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could do as we do with optional dependencies, i.e., having a function that evaluates if this is needed on the fly based on othere available extensions?

@chrishalcrow

chrishalcrow commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Hey @ecobost , I went through your comments on #4374 and mostly resolved them (sorry it's on a new PR, I didn't have permission to push to Sam's branch). Here's a list of "controversial" things to discuss.

  • I think I've been too hrash getting rid of peak_sign and peak_mode in estimate_sparsity. This was actually used for Templates sensibly... I should re-insert... I'll do this soon!!
  • @ecoboost pointed this out: at the moment, the main_channel_indices are a sorting property, and (ideally) the analyzer gets it from there. But in many cases the analyzer computes the main_channel_indices, and it's then set as a sorting property. In this case, the sorting inherits from the analyzer, which sounds bad. How to resolve?
  • estimate_sparisty default is now "both", matching create_sorting_analyzer. It's only used in create_sorting_analyzer
  • @ecobost suggests removing sparsity_kwargs from estimate sparsity. I'm against this, but mostly because I use them, which isn't a great reason.
  • It's not clear if we should allow a user to pass their own "peak_sign" to e.g. get_template_amplitudes, because this is usually a bad idea. I suggest adding a override_peak_error flag, so that expert users have to turn this on to mess with it. I think @alejoe91 prefers to just allow the user to pass it.
  • I've added peak_sign_for_signal and peak_mode_for_signal in compute_snrs. I think these have use cases: I can imagine people wanting to compute the peak_to_peak value on main_channel.
  • At the moment, we're deprecating get_template_extremum_amplitude, get_template_extremum_channel_peak_shift, get_template_extremum_channel in favour of similarly named functions (but with main_channel). I'm a bit worried about this - our lab uses get_template_extremum_channel in various places and I'd bet it's used by other developers. Maybe we should just keep the old naming convention and use extremum_channel_indices. Or just keep get_template_extremum_channel for a while??

@samuelgarcia @alejoe91 could you take a look at this - would be great to get merged next week!!

alejoe91
alejoe91 reviewed Jun 29, 2026
View reviewed changes
Comment thread src/spikeinterface/core/basesorting.py
"""

_main_properties = [
"main_channel_index",

@alejoe91 alejoe91 Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's be careful because channel indices could change (e.g., build analyzer with/without bad channels).. how should we deal with it?

alejoe91
alejoe91 reviewed Jun 29, 2026
View reviewed changes
Comment thread src/spikeinterface/core/sortinganalyzer.py
recording,
format="memory",
folder=None,
main_channel_index=None,

@alejoe91 alejoe91 Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
main_channel_index=None,
main_channel_indices=None,

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alejoe91 alejoe91 alejoe91 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

core Changes to core module postprocessing Related to postprocessing module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chrishalcrow @alejoe91 @samuelgarcia